[APP-14487] Update viam-agent to parse and authenticate with API keys #176
Conversation
danielbotros
force-pushed
the
APP-14487-update-agent-to-priortize-api-key-creds
branch
from
97a9c95 to
b8f6c18
Compare
danielbotros
changed the title
| type APIKey struct { | ||
| ID string `json:"id"` | ||
| Value string `json:"value"` | ||
| } |
There was a problem hiding this comment.
Should this say "key" instead of "value" to be consistent with https://github.com/viamrobotics/app/pull/10703?
There was a problem hiding this comment.
Yeah good catch thank you!
| return | ||
| } | ||
| if cfg.Cloud == nil || (cfg.Cloud.ID == "" || cfg.Cloud.Secret == "" || cfg.Cloud.AppAddress == "") { | ||
| if cfg.Cloud == nil || (cfg.Cloud.ID == "" || (cfg.Cloud.Secret == "" || cfg.Cloud.APIKey.IsEmpty()) || cfg.Cloud.AppAddress == "") { |
There was a problem hiding this comment.
Not sure what this function does and I see that we still have n.portalData.input.Secret = cloud.GetSecret() in the other file, but do we indeed want to require that both a secret and API key is present here?
Mostly wondering about the intention since it goes against how you tested connecting and we're using && in parseOpts.
There was a problem hiding this comment.
I don't have a strong opinion of requiring both, but the intention was just to check that at least one of API key or secret exists in the cloud config so if secrets ever get removed this code doesn't need to be updated.
The logic in parseOpts is a bit hard to read so I might try to refactor it but that is enforcing that if one cloud config field is present then we must have a fully valid cloud config (that is, at least of API key or secret exists), but an empty cloud config is also ok. Whereas these checks enforce that we always have a complete cloud config (empty not ok), completing meaning at least one of API key or secret.
There was a problem hiding this comment.
completing meaning at least one of API key or secret.
Right, I think right now we're returning an error if one is missing here. It'd have to be && for us to allow one to be missing.
There was a problem hiding this comment.
Oops yeah you are right. I was thinking these if-statements were the allowable conditions not the error condition.
| } | ||
|
|
||
| if cfg.Cloud.AppAddress == "" || cfg.Cloud.ID == "" || cfg.Cloud.Secret == "" { | ||
| if cfg.Cloud.AppAddress == "" || cfg.Cloud.ID == "" || (cfg.Cloud.Secret == "" || cfg.Cloud.APIKey.IsEmpty()) { |
There was a problem hiding this comment.
(same as my other question, probably gonna be the same answer)
cmd/provisioning-client/opts.go
Outdated
| AppAddr string `default:"https://app.viam.com:443" description:"Cloud address to set in viam.json" long:"app-addr"` | ||
| PartID string `description:"PartID to set in viam.json" long:"part-id"` | ||
| Secret string `description:"Device secret to set in viam.json" long:"secret"` | ||
| APIKey utils.APIKey `description:"Device secret to set in viam.json" long:"api-key"` |
There was a problem hiding this comment.
did you test the bt provisioning flow and how did you do it?
There was a problem hiding this comment.
@cheukt I was able to test the bt provisioning flow and ran it by mobile (Clint). What I did is I set my agent into bluetooth provisioning mode and paired it with my iPhone. On my phone, I installed an app called LightBlue which lets me interact with low level bluetooth details for connected devices . I was able to inspect my bluetooth connection to my Pi and could read/write bluetooth characteristics from my phone to the Pi. I matched each characteristic UUID to its characteristic key name and I read the cryptoKey and used that to successfully encrypt and write an APIKey struct to my Pi over bluetooth.
Clint also mentioned there should be one more ticket separate from this agent work to add this characteristic to mobile's Flutter wrapper for provisioning/bluetooth which will allow me to do more testing easier which I will pair with Kevin tomorrow for that.
danielbotros
force-pushed
the
APP-14487-update-agent-to-priortize-api-key-creds
branch
from
de34b48 to
5152f2f
Compare
3 participants
Description
APP-14487 Update viam-agent to parse and authenticate with API keys
Cloudpart of the config in order to parse and validate API keys in order to use them as the primary auth credentials moving forward, using secrets as a fallback.Testing
Config used / manually modified: